Conversation
dossett
reviewed
Aug 17, 2026
dossett
reviewed
Aug 17, 2026
Member
Author
|
cc @wgtmac can you take a look? |
wgtmac
reviewed
Aug 28, 2026
wgtmac
reviewed
Sep 23, 2026
Track original filesystem allocations, including checksum buffers and buffers backing returned slices, and release them with the row group. Clean up the row group when reading or parsing fails. Include submission and every requested range in a single read deadline. Invalidate failed readers and defer stream cleanup until submission exits, without delaying interruption or recycling buffers still used by IO. Publish available futures after partial submission failures and fall back to ordinary reads when a pre-submission file-length lookup fails. Add regression coverage for buffer ownership, submission failures, timeouts, interruption, and metadata fallback.
sunchao
force-pushed
the
dev/chao/codex/gh-3719-vectored-read-safety
branch
from
September 23, 2026 16:27
f82eaa4 to
5d3d6f3
Compare
wgtmac
reviewed
Sep 24, 2026
Member
|
Do you want to take a look as well? @gszadovszky @Fokko @divjotarora |
Contributor
|
@wgtmac I'd be interested in reviewing, but I'm a bit swamped right now. Can get to it tomorrow if that's OK |
divjotarora
approved these changes
Sep 25, 2026
divjotarora
left a comment
Contributor
There was a problem hiding this comment.
Overall looks good, thanks for the investigation and thorough fix @sunchao!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why are the changes needed?
The Hadoop vectored-read path can fall back to ordinary reads after filesystem
requests have been submitted or results partially consumed. Replaying reads into
the same chunk builder can duplicate selected page data and return incorrect
filtered rows. Outstanding requests also need to retain ownership of their stream
and buffers during failure cleanup.
The current path requests a single buffer for an entire contiguous projected
range, without splitting it at
parquet.read.allocation.size.What changes were proposed in this PR?
logical page/column plan. Verify dictionary and V1/V2 page checksums across split
buffers without another contiguous checksum allocation.
submission disables vectored IO for that reader and permits ordinary reads.
the interface cannot reliably distinguish rejection from partial submission.
once through the JVM system property
parquet.hadoop.vectored.io.threads.Admission, submission, and range waits share one 300-second deadline. Admission
failure leaves the stream with its reader. Accepted operations retain capacity
until submission exits, the caller transfers or abandons buffer ownership, and
failure cleanup finishes. Cancellation cannot free capacity prematurely.
Idle workers expire after 60 seconds.
and transfer them to the row-group releaser on success. Preserve automatic
row-group cleanup on sequential reads and reader closure.
readers and defer stream cleanup until submission exits; release buffers only
when completion can be established. Guarantee abort with
finallyeven whendraining sibling reads throws an
Error, and retain the original failure assuppressed on the escaping error. Remove the unused CRC allocator.
The allocation setting bounds requested range lengths, not every filesystem
allocation: backends may merge ranges or align them for checksums and allocate
larger buffers. It does not bound footer/decoder allocations or total memory.
The worker limit does not bound threads created by the filesystem. A backend
that ignores interruption, or a blocked stream close, can retain capacity
indefinitely; when all workers are occupied, further reads time out at admission.
Missing, cancelled, or never-completing result futures can prevent safe buffer
reclamation.
How was this PR tested?
Validated the current implementation on Java 17.0.20.1 with Maven 3.9.16 and
Thrift 0.24.0:
parquet-hadoopmodule: 869 tests, zero failures/errors, 26Hadoop-capability skips with default Hadoop 3.3.0. Spotless passed; RAT
approved 273 licenses.
-Dparquet.hadoop.vectored.io.threads=2: 110 tests, zerofailures/errors/skips, including the real local-filesystem vectored and
file-range bridge tests.
finallyguards makes all foursibling-error ownership cases fail; the guards were restored before both
successful runs above.
compatibility checks, passed.
git diff --checkpassed.The focused run selected
TestParquetFileReaderVectoredIO,TestParquetFileReaderVectoredOwnership,TestVectoredReadBufferAllocator,TestVectoredReadOperation,TestDataPageChecksums,TestVectorIoBridge,TestFileRangeBridge,TestParquetFileReaderBufferLeak.New coverage includes saturated admission across repeated retries, capacity
retention during blocked submission and stream closure, interruption before
admission with reader reuse, abort before the worker starts, rejection without
lost capacity, and sibling errors with pending reads and delayed buffer release.
Local setup used a test-JVM hosts file for the machine hostname and interop
fixtures fetched at the commits pinned in the tests. No live S3 validation or
performance benchmark was performed.
Closes #3719.